-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] Sticky headers #10059
[DataGrid] Sticky headers #10059
Conversation
Scrolling is so smooth 🧈
Same as for pinned column: #5979. It's probably OK. At least, as a developer, I don't mind the pain to customize this, in exchange of the smoother UX. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Isn't this much better? Before: https://deploy-preview-11650--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions Screen.Recording.2024-01-22.at.16.15.45.movAfter: https://deploy-preview-10059--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions Screen.Recording.2024-01-22.at.16.14.18.mov |
@oliviertassinari, try disabling overscroll-behavior, and will look and feel even more performant on iOS (as I pointed out here: #11230 (comment)). You wont have all the rows disappearing for a brief moment when you accidentally overscroll on the X-axis while actually just trying to scroll down ;) |
@romgrk I found a regression, this PR broke this demo: https://next.mui.com/x/react-data-grid/row-updates/#lazy-loading
@lauri865 I would expect #11230 to solve this. It also sounds like a performance issue, I don't get less white screen on AG Grid. For |
Opened the issue linked above. |
@romgrk In terms of scrolling performance, I have noticed a few things on this PR. First, the baseline, ranked by how much time it takes to render the next rows, the difference is clear on my laptop (in production and also more noticeable in dev mode):
Now, when we look at the actual profile for 1,000 ms of scroll, we see a much different story:
We were 1st Before, and clearly, I think the most important metric to judge performance is idle time. However, because we are not using a high enough overscan, end-users would feel a white area. It seems clear to me that 3 is not enough https://mui.com/x/api/data-grid/data-grid/#DataGrid-prop-rowBuffer. Increasing it would be a perfect quick win. Another opportunity I see is how we re-render emotion components at each render, e.g. ScrollbarFiller. How about we memoize or move the style outside of the re-render flow during scroll? Now, the problem. Vertical scrolling is a more frequent use case than Horizontal scrolling, so should be prioritized in terms of UX if we can only have one. So I think for this PR to benefit end-users, we need to get to the bottom of this regression. I suspect we can fix it. If we can't, and it's because of a core tradeoff of the sticky position, then I think it would be better to stay on the previous side of the tradeoff (not using it). We are no longer ahead of AG Grid, more like a tie now (once we increase the overscan and save Emotion rendering cost). So, I drilled deeper, there is a x2 increase in "Rendering" time in After compared to Before. This seems to come from two things:
As I can see it, it's because of this style: Screen.Recording.2024-01-22.at.17.16.13.movOnce this style removed, the computation bottleneck seems to then move to recalculateStyles, this is very odd to me: More in context: I don't know why. I ran out of time I can allocate to this 😄 |
Yes I've noticed the performance changes, I merged anyway because I wanted to get the breaking changes in. I kinda expected the layout to be more expensive for the browser due to that I also initially used CSS variables a lot in this PR, often in dynamic ways (e.g. Regarding emotion, that library is absolutely atrocious for runtime performance and the sooner we can get rid of it the happier I will be. I have been trying to find a way to get emotion to do something like this: const className = css` color: red ` instead of having to use their styled-components, but I don't know the library and our system style wrappers much so I also wasn't able to complete this during this PR. Discussing how we do styling internally on the datagrid is on my roadmap. Lastly regarding the row overscan, I've already opened #11344 to discuss that aspect, read there for more details. |
@romgrk Alright, let's continue to explore this then. Overall, it's not yet clear that we should sell it as a performance improvement in the v7 release blog post, but it feels like we are close 😁
A nested CSS selector from the parent does something close, as long as that parent doesn't rerender. We moved many styles up to the grid root in the past because of this.
I don't know if they increase layout time, I wouldn't expect them to. |
That's terrible. We don't get the CSS-in-JS advantage of colocation of styles & logic, and we don't get the CSS advantage of native (clean) CSS syntax. I've been observing I've been watching the zero-runtime solution in the hopes that it solves all our style issues, in the meantime if there's any solution that would help with injecting CSS simply & directly, I'd definitely adopt that. |
Extracted vertical scrolling performance to a separate issue: #11866 |
I've opened #11924 for the performance issues. |
Last fix for the issues above opened in #12019. CSS variables were the culprit. |
Closes #4506
Closes #9646
Closes #5416
Closes #10616
Closes #10339
Closes #11214
Closes #11215
Closes #9502
Closes #11691
Implement
position: sticky
grid headers. It's based on the work in #9589.Before: https://deploy-preview-11650--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions
After: https://deploy-preview-10059--material-ui-x.netlify.app/x/react-data-grid/#commercial-versions
Before
header tearing, header not responding to scroll intent, white screen areas
Screen.Recording.2023-12-11.at.16.34.30.mov
After
no header tearing, header responding to scroll intent, trade no white screen areas for scrollbar tearing
Screen.Recording.2023-12-11.at.16.39.11.mov
Stuff that needs to change:
Remaining tasks:
XXX:
note=> We use the colors from the theme.